feat: add OpenAI Agents SDK support to OpenInference mapper - #366
feat: add OpenAI Agents SDK support to OpenInference mapper#366liramon2 wants to merge 14 commits into
Conversation
|
@strandly-the-agent Review this PR. Consolidate findings into a single comment. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@strandly-the-agent Here are the instrumentation:
Here are the current findings: For OpenAI agents SDK + traceloop instrumentation:
For OpenAI agents SDK + OpenInference instrumentation:
NOTE: In the current mappers' implementation strategy, I don't think we should use composition, and inheritance. And the mappers are grouped by instrumentation frameworks. I need you to:
|
|
Verified all seven claims against the instrumentation sources (
Design answer (Q2/Q3): agree — no inheritance, and no new classes. Traceloop verification (T1–T4)T1 — GenAI conventions ✅. Fixture chat spans carry T2 —
T3 — ✅ confirmed in both fixtures. Root T4 — ✅ confirmed. In the ADOT fixture,
OpenInference verification (O1–O3)O2 — mechanism ✅, paths and indices need precision. Verified against the v2.0.0 instrumentor source and both #366 fixtures:
O3 — ✅ verified, with one important limit.
Design recommendation (Q2/Q3): no new classes, no inheritance — three named phases inside the two family mappersThe rule that matches the repo you already have: one mapper per emitting-convention family ( Why I'd agree about inheritance specifically: #365's subclass works by overriding The shape I'd standardize (both mappers already approximate it):
Three guardrails, all from bugs already found in these two PRs: each hook is exception-isolated (one producer's quirk must degrade to a warned-and-skipped trace, not kill the session — the #366 normalizer currently runs outside every try/except); each hook checks scope on the spans it touches (the #366 walk doesn't); and each hook gets producer fixtures that pin it (mutation testing showed the Applied to the two PRs:
One thing I'd not do: don't introduce a producer-level abstraction (base class or protocol for "producers") until a third family needs it. Two call sites is a pattern; codifying it early is how the mapper layer gets rigid. Verification by |
|
@strandly-the-agent Review this PR. Consolidate findings into a single comment. For your questions:
|
This comment was marked as resolved.
This comment was marked as resolved.
|
@strandly-the-agent Review this PR. Consolidate findings into a single comment. |
This comment was marked as resolved.
This comment was marked as resolved.
dc61839 to
4717876
Compare
4717876 to
09f67b9
Compare
09f67b9 to
3158379
Compare
3158379 to
fe9c5d8
Compare
dbbbd53 to
ccc3086
Compare
|
@strandly-the-agent Review this PR (resolved merge conflicts + added test since last review). Consolidate findings into 1 comment. |
|
Re-reviewed at All clear — the round-4 🟡 is closed, the rebase is drift-free, and I have no findings.
Follow-up review by |
| - openinference-instrumentation-langchain (LangChain / LangGraph) | ||
| - openinference-instrumentation-smolagents (HuggingFace smolagents) | ||
| - openinference-instrumentation-claude-agent-sdk (Claude Agent SDK) | ||
| - openinference-instrumentation-openai-agents (OpenAI Agents SDK) |
There was a problem hiding this comment.
how do we pick up this naming? Wondering claude-agent-sdk but for openai agent sdk we only do openai-agents
There was a problem hiding this comment.
These are the official package names from the OpenInference repository: https://github.com/Arize-ai/openinference/tree/main
| return llm_spans | ||
|
|
||
| @staticmethod | ||
| def _get_message_indices(attrs: dict, prefix: str) -> list[int]: |
There was a problem hiding this comment.
Nit: overall, can we make function naming more accurate
There was a problem hiding this comment.
Done in 8bf879e. Standardized the verbs from get to extract/collect.
| indices.add(int(seg)) | ||
| return sorted(indices, reverse=True) | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
Do these helpers need class-level invocation or subclass override behavior? Origin code is just some private helpers.
if they are a must, can we make code more consistent?
There was a problem hiding this comment.
They don't need access to self, but then again other private helpers in the file don't access self. So to answer your question, they can just be private helpers. Done in 8bf879e
|
@strandly-the-agent Review this PR. Consolidate any findings into 1 comment. |
|
Re-reviewed at One 🟡 to flag: the “stale” error check wasn't stale.
🟡 the removed
|
This reverts commit 02a76ed.
Description
Add OpenAI Agents SDK support to OpenInference mapper. This handles both Cloudwatch ADOT and in-memory exporter formats. No inheritance or composition is used because that is a one-way door decision that calls for a larger refactor.
This also fixes parent span ids in
OpenInferenceSessionMapperso that they point to converted spans. This preserves the agent-tool scopes when converting from OpenAI Agent traces (or other OpenInference traces) to evaluator inputs.Related Issues
#364
Documentation PR
Type of Change
New feature
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.